-
Notifications
You must be signed in to change notification settings - Fork 300
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Serialization support for FlavorResource, FlavorResourceSet and FlavorResourceQuantities #4420
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nasedil The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @nasedil! |
Hi @nasedil. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
frsNeedPreemption sets.Set[resources.FlavorResource] | ||
frsNeedPreemption FlavorResourceSet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think we should do for logging. Instead of that, why we can not just use slice?
frsNeedPreemption.UnsortedList()
@mimowo Did you indicate JsonMarshal proposed in this PR? If yes, what is the reason not to use slice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, my suggestion was to use slice rather than Json, for sure. I was just thinking about exposing this via String function, but frsNeedPreemption.UnsortedList()
is also neat. I'm ok either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually yes, it looks like logger manages to marshal frsNeedPreemption.UnsortedList()
just fine, and using String()
for FlavorResourceSet
I still need to do marshalling somehow. So I can substitute frsNeedPreemption
with frsNeedPreemption.UnsortedList()
in klog calls just fine. Moreover, it works even when FlavorResource
and FlavorResourceQuantities
do not implements String()
.
However the line
kueue/pkg/scheduler/logging.go
Line 43 in 398c74d
logV.Info("Workload evaluated for admission", args...) |
"json: unsupported type: resources.FlavorResourceQuantities"
error (even when FlavorResource
and FlavorResourceQuantities
implement String()
). And even if I implement JsonMarshal for them it would not output as nicely as it works . Looks like there is some functionality in klog that nicely formats to json that should be used, not sure why it doesn't pick up in this case.
I also see klog.KObj(preemptionCtx.preemptor.Obj)
in klog calls, maybe this could be relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still leads to the "json: unsupported type: resources.FlavorResourceQuantities" error (even when FlavorResource and FlavorResourceQuantities implement String())
Yeah, this is surprising - I was assuming adding String() for resources.FlavorResourceQuantities
would be enough, as the logger should check the input type implementes the Stringer interface and call String(), we had a similar case here: kubernetes/kubernetes#121392 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can trace that e.assignment
is passed to the logger here:
kueue/pkg/scheduler/logging.go
Line 40 in 398c74d
args = append(args, "nominatedAssignment", e.assignment) |
where
e
is of type entry
.
The type entry
is defined here:
kueue/pkg/scheduler/scheduler.go
Lines 320 to 331 in 398c74d
type entry struct { | |
// workload.Info holds the workload from the API as well as resource usage | |
// and flavors assigned. | |
workload.Info | |
dominantResourceShare int | |
dominantResourceName corev1.ResourceName | |
assignment flavorassigner.Assignment | |
status entryStatus | |
inadmissibleMsg string | |
requeueReason queue.RequeueReason | |
preemptionTargets []*preemption.Target | |
} |
And e.assignment
is of type Assignment
from here:
kueue/pkg/scheduler/flavorassigner/flavorassigner.go
Lines 43 to 54 in 398c74d
type Assignment struct { | |
PodSets []PodSetAssignment | |
Borrowing bool | |
LastState workload.AssignmentClusterQueueState | |
// Usage is the accumulated Usage of resources as pod sets get | |
// flavors assigned. | |
Usage resources.FlavorResourceQuantities | |
// representativeMode is the cached representative mode for this assignment. | |
representativeMode *FlavorAssignmentMode | |
} |
which uses
FlavorResourceQuantities
which are type FlavorResourceQuantities map[FlavorResource]int64
. And FlavorResource
is serialized fine in frsNeedPreemption.UnsortedList()
.
Maybe logger supports structs like:
type FlavorResource struct {
Flavor kueue.ResourceFlavorReference
Resource corev1.ResourceName
}
But not map[FlavorResource]int64
out of the box? That's just one guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this helps much, but you can setup also debugger in vs-code for the integration tests.
My setup for the settings.json in vs-code you just need to set:
"go.testEnvVars": {
"KUBEBUILDER_ASSETS": "/usr/local/..**../kubebuilder-envtest/k8s/1.28.3-linux-amd64",
},
The full value of the var for your setup should be printed when invoking make test-integration
target.
/ok-to-test |
The wrapper makes logger calls with sets.Set[resources.FlavorResource] serializable, without changing sets.Set upstream.
446bae6
to
a3a7883
Compare
Removed redundant FlavorResourceSet type and associated marshaling methods because `klog` manages to serialize `frsNeedPreemption.UnsortedList()` properly.
a3a7883
to
64eed78
Compare
What type of PR is this?
What this PR does / why we need it:
Properly serializes FlavorResource, FlavorResourceSet and FlavorResourceQuantities types which are logged in integration tests.
Which issue(s) this PR fixes:
Fixes #4137
Special notes for your reviewer:
As discussed in #4137 part of this issue depends on making serializable
k8s.io/apimachinery/pkg/util/sets.Set
in upstream repo. In the second commit I createdFlavorResourceSet
type to make it work without changing upstream code. Possible options are to keep it temporarily until upstream is fixed, keep it permanently, or just discart and keep only the first commit and wait till upstream issue is fixed.Does this PR introduce a user-facing change?